Skip to content

Add support for FIPS Updates images #483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 29, 2025
Merged

Conversation

renanrodrigo
Copy link
Member

@renanrodrigo renanrodrigo commented Apr 11, 2025

PR Checklist

  • I have added unit tests to cover the new behavior under ``tests/unit_tests/```
  • I have run tox -e format locally to automatically format my code before submitting
  • I have run tox locally ensuring that it passes before submitting
  • (if applicable) I have added a reference to issues that this PR relates to in the PR message (Refs GH-1234, Fixes GH-1234)
  • My commits are atomic and follow the convetional commit message format (https://www.conventionalcommits.org/en/v1.0.0/)

Description

This PR adds a new ImageType: PRO_FIPS_UPDATES.
Clouds are now shipping fips-updates images instead of the regular fips ones.

On EC2, Focal has both, so we need a way to tell them apart. There will be Focal fips-updates images on the other clouds soon too - on GCE they can be differentiated by the name filter, and Azure needs to be added manually.

For all clouds, this also adds the necessary filters/identifiers to launch Jammy fips-updates images.

Fixes: #481
Fixes: #482

Additional Context and Relevant Issues

N/A

Test Steps

Run the demo! Check that fips is enabled on the FIPS image, and fips-updates is enabled on the FIPS Updates image.

Clouds now have images with FIPS Updates instead of the regular FIPS
ones. Focal has both, so there is a need to differentiate.

Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
@renanrodrigo renanrodrigo changed the title Add fips updates ec2 Add support for FIPS Updates images on EC2 Apr 11, 2025
@a-dubs
Copy link
Contributor

a-dubs commented Apr 13, 2025

changes look good!

Wrote an integration test to verify this works as expected. Will approve once you add it:

diff --git a/tests/integration_tests/ec2/__init__.py b/tests/integration_tests/ec2/__init__.py
new file mode 100644
index 0000000..ea323f9
--- /dev/null
+++ b/tests/integration_tests/ec2/__init__.py
@@ -0,0 +1 @@
+"""EC2 integration tests."""
diff --git a/tests/integration_tests/ec2/test_images.py b/tests/integration_tests/ec2/test_images.py
new file mode 100644
index 0000000..16720aa
--- /dev/null
+++ b/tests/integration_tests/ec2/test_images.py
@@ -0,0 +1,51 @@
+"""EC2 integration tests testing image related functionality."""
+
+import logging
+
+import pytest
+
+from pycloudlib.cloud import ImageType
+from pycloudlib.ec2.cloud import EC2
+
+logger = logging.getLogger(__name__)
+
+
+@pytest.fixture
+def ec2_cloud():
+    """
+    Fixture to create an EC2 instance for testing.
+
+    Yields:
+        EC2: An instance of the EC2 cloud class.
+    """
+    with EC2(tag="integration-test-images") as ec2:
+        yield ec2
+
+
+def test_finding_all_image_types_focal(ec2_cloud: EC2):
+    """
+    Tests that all image types are available for the focal suite and that they are all unique.
+
+    As per issue #481, focal has both `fips` and `fips-updates` image types and previous to
+    introducing the `PRO_FIPS_UPDATES` image type, the `PRO_FIPS` image type could return a
+    `PRO_FIPS_UPDATES` image if it was newer. This test asserts that PR #483 prevents this from
+    happening.
+
+    Test assertions:
+    - All image types are available for the focal suite (exception is raised if not).
+    - No daily images returned per image type are the same (same image ID).
+    """
+    suite = "focal"
+    images: dict[ImageType, str] = {}
+    # iterate through all ImageType enum values
+    for image_type in ImageType:
+        images[image_type] = ec2_cloud.daily_image(release=suite, image_type=image_type)
+        logger.info(
+            "Found %s image for %s: %s",
+            image_type,
+            suite,
+            images[image_type],
+        )
+
+    # make sure that none of the images are the same
+    assert len(set(images.values())) == len(images), f"Not all images are unique: {images}"

@a-dubs
Copy link
Contributor

a-dubs commented Apr 14, 2025

@rthill91 @rpocase I would appreciate it if someone from AWS squad could review this when they have a moment given it is EC2 specific. thanks in advance 💙

Copy link

@rthill91 rthill91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments regarding variant names, but otherwise lgtm

Fixes: #481

Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
Fixes: #482

Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
Update the three clouds' demos to launch FIPS Updates images, using
series where it is available.

Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
Signed-off-by: Alec Warren <alecwarren19@gmail.com>
Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
@renanrodrigo renanrodrigo force-pushed the add-fips-updates-ec2 branch from 08e0059 to a38b5bd Compare April 15, 2025 04:50
@renanrodrigo renanrodrigo changed the title Add support for FIPS Updates images on EC2 Add support for FIPS Updates images Apr 15, 2025
@renanrodrigo
Copy link
Member Author

@a-dubs @rthill91 Thanks much for the initial reviews!

  • I have changed the string on ec2 to not include the *
  • I have added the suggested integration test

I was looking at the PR and decided to add the functionality to the other clouds too - I created separate issues, but given each PR here is a version bump, it would make sense to me that the whole functionality lands with a single version bump.
So now this PR has, besides the ec2 logic, the gce logic and the azure entry for jammy fips-updates.

@a-dubs besides the test you suggested, do you think something similar would be needed to verify images are fetched correctly from gce? I think Azure doesn't need an integration tests as the p:p:ps are hardcoded anyway :D

@a-dubs
Copy link
Contributor

a-dubs commented Apr 16, 2025

I think it would be foolish to not add integration tests given the work already put in. Here's my implementation of a GCE integration test. Still not sure if something so hardcoded is the best approach if images become available for jammy/focal as time passes but for now this is nice to have because it codifies the current functionality we see/expect 🤷‍♂️

diff --git a/tests/integration_tests/gce/test_images.py b/tests/integration_tests/gce/test_images.py
new file mode 100644
index 0000000..3391b74
--- /dev/null
+++ b/tests/integration_tests/gce/test_images.py
@@ -0,0 +1,73 @@
+"""GCE integration tests testing image related functionality."""
+
+import logging
+
+import pytest
+
+from pycloudlib.cloud import ImageType
+from pycloudlib.errors import ImageNotFoundError
+from pycloudlib.gce.cloud import GCE
+
+logger = logging.getLogger(__name__)
+
+
+@pytest.fixture
+def gce_cloud():
+    """
+    Fixture to create a GCE instance for testing.
+
+    Yields:
+        GCE: An instance of the GCE cloud class.
+    """
+    with GCE(tag="integration-test-images") as gce:
+        yield gce
+
+@pytest.mark.parametrize(
+    "release, unavailable_image_types",
+    (
+        pytest.param(
+            "focal",
+            [ImageType.PRO_FIPS_UPDATES],
+            id="focal",
+        ),
+        pytest.param(
+            "jammy",
+            [ImageType.PRO_FIPS],
+            id="jammy",
+        ),
+    ),
+)
+def test_finding_all_image_types_focal(
+    gce_cloud: GCE,
+    release: str,
+    unavailable_image_types: list[ImageType],
+):
+    """
+    Tests that all image types are available for the focal suite and that they are all unique.
+
+    Test assertions:
+    - Certain image types are unavailable for the given release (exception is raised if not).
+    - No daily images returned per image type are the same (same image ID).
+    """
+    images: dict[ImageType, str] = {}
+    # iterate through all ImageType enum values
+    for image_type in ImageType:
+        if image_type in unavailable_image_types:
+            with pytest.raises(ImageNotFoundError) as exc_info:
+                gce_cloud.daily_image(release=release, image_type=image_type)
+            logger.info(
+                "Confirmed that %s image for %s is unavailable.",
+                image_type,
+                release,
+            )
+        else:
+            images[image_type] = gce_cloud.daily_image(release=release, image_type=image_type)
+            logger.info(
+                "Found %s image for %s: %s",
+                image_type,
+                release,
+                images[image_type],
+            )
+
+    # make sure that none of the images are the same
+    assert len(set(images.values())) == len(images), f"Not all images are unique: {images}"

@a-dubs
Copy link
Contributor

a-dubs commented Apr 16, 2025

@renanrodrigo I will leave it up to you if you want to do an azure test or not because yes the implementation is hilariously hard coded like you mentioned. I don't feel like setting up an azure account just to test this so I have no way of testing it so I shall leave it up to you. should be simple enough to convert this GCE one to an azure one 🤷‍♂️

Cheers! Thanks for all your hard work on this.

Copy link
Member

@toabctl toabctl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good from an AWS/EC2 perspective.

Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
Signed-off-by: Renan Rodrigo <renanrodrigo@canonical.com>
@renanrodrigo renanrodrigo force-pushed the add-fips-updates-ec2 branch from a38b5bd to f47a3c0 Compare April 23, 2025 19:01
@renanrodrigo
Copy link
Member Author

@a-dubs added the suggested intgration test, not adding the Azure one for now.
@rthill91 All should be good to go now?

thanks!

Copy link
Contributor

@a-dubs a-dubs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heck yeah good stuff @renanrodrigo. thanks for all your work on this. LGTM 🚀

@a-dubs
Copy link
Contributor

a-dubs commented Apr 27, 2025

@rthill91 poking you for re-review 🫡

@a-dubs
Copy link
Contributor

a-dubs commented Apr 29, 2025

@renanrodrigo fire in the hole!

@renanrodrigo renanrodrigo merged commit 7e7f67b into main Apr 29, 2025
7 of 8 checks passed
@renanrodrigo renanrodrigo deleted the add-fips-updates-ec2 branch April 29, 2025 17:25
@renanrodrigo
Copy link
Member Author

HEEEEEY thanks everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Need a way to spin pro-fips-upates instances on Azure, GCE Wrong image returned when querying Pro Fips images on EC2
4 participants